Improve Scene Tree editor performance #99700

hpvb · 2024-11-26T01:39:49Z

We now cache the Node*<>TreeItem* mapping in the SceneTreeEditor. This allows us to make targeted updates to the Tree used to display the scene tree in the editor.

Previously on almost all changes to the scene tree the editor would rebuild the entire widget, causing a large number of deallocations an allocations. We now carefully manipulate the Tree widget in-situ saving a large number of these allocations.

There is definitely more that could be done, but this is already a massive improvement.

This fixes #83460

hpvb · 2024-11-26T01:41:08Z

Before: (this scene has about 15,000 nodes)

Screencast.From.2024-11-26.02-11-22.mp4

After:

Screencast.From.2024-11-26.02-10-35.mp4

arkology

Stealing @AThousandShips 's job 🤣

editor/gui/scene_tree_editor.cpp

editor/gui/scene_tree_editor.h

scene/gui/tree.cpp

editor/gui/scene_tree_editor.cpp

arkology

I confirm that birdies are in the nest (dots at the end of the comments)😃

editor/gui/scene_tree_editor.cpp

KoBeWi · 2024-11-26T13:25:12Z

Changing valid types does not update the nodes (they stay disabled/enabled).

Can be tested with this simple EditorScript:

extends EditorScript
func _run() -> void:
	EditorInterface.popup_node_selector(func(n): print(n), ["Node2D"])

Run it, then remove "Node2D" and run again. Nodes that didn't match originally will stay disabled.

editor/gui/scene_tree_editor.cpp

doc/classes/TreeItem.xml

hpvb · 2024-11-28T01:21:15Z

@KoBeWi I think I addressed your issues. Part of the reason I didn't figure it out was that there's a bug in master already whereby in the reparent node dialog nodes with disabled processing wouldn't show up as red. This is also fixed in this PR now.

The following things were tested:

old tests:

Toggling unique names / undo
Make local / undo
Configuration warning / solve warning / undo
Adding to group / undo / Adding to group while connected / undo
Connecting a signal / undo/ Connecting signal while in a group / undo
Editable children / undo
Reparenting node / undo
Reordering node / undo
Change editor theme / and back
popup_node_selector (your test case)

new tests:

Reparenting node via dialog, verified the colors and behavior against master
Creating connections via connections dialog, verified colors and behavior against master
Adding a script / undo
Make local / undo (verify icon/tooltip state)
Create new node in an "editable children" scene, verify colors match master
Toggle lock/unlock/undo, verify that lock icon appears in the scene tree
Verify that AnimationPlayer pin matches behavior on master
Set a node process mode / undo
Toggle unique name / undo
Reparent node via dialog, with the nodes having processing disabled. Verify that they still turn red (unlike on master)
Reparent node, cancel out of dialog, reparent a differnt node, validate that nodes do not stay red

KoBeWi · 2024-11-28T14:38:41Z

Found another bug with node selector.
Changing type now properly changes colors, but the nodes stay unselectable.

editor/gui/scene_tree_editor.h

editor/gui/scene_tree_editor.cpp

KoBeWi · 2024-11-28T15:08:40Z

I checked the implementation and looks like the main idea is hashing everything relevant for node appearance and using it to update only when needed. This means that every time we add something new that can affect anything about TreeItems we need to add it to the hash; rather error-prone, but the improved performance makes this compromise reasonable.
Though there are still some properties tracked outside of the hash. I wonder if they could be included too.

There are still a couple of bugged cases, but other than that I think the PR is fine overall.

hpvb · 2024-11-28T15:14:05Z

I checked the implementation and looks like the main idea is hashing everything relevant for node appearance and using it to update only when needed. This means that every time we add something new that can affect anything about TreeItems we need to add it to the hash; rather error-prone, but the improved performance makes this compromise reasonable. Though there are still some properties tracked outside of the hash. I wonder if they could be included too.

As for changing the hash, I agree that it is a bit more work but on the other hand, for new things added it not showing up in the editor should be immediately apparent, unlike now where we are adding it now.

The things checked outside of the hash are things that are related to the editor state, not the node state. I didn't want to have to pass variables into the _hash_node() method. I could make it work that way if you'd like? It'd be pretty much the same amount of code.

For the pre_filter_color, that cannot be part of the hash as it is something that happens after nodes are updated and is also purely editor state.

There are still a couple of bugged cases, but other than that I think the PR is fine overall.

Nodes stay unselectable in the filter screen after changing the node type
Renaming a group doesn't get reflected on the tooltip

I think that is all you found this round, right?

KoBeWi · 2024-11-28T15:23:06Z

Yes, last time I just checked all code that sets custom color or adds buttons, but there are other ways how TreeItems can be changed. There might be some tooltip changes I missed.

The things checked outside of the hash are things that are related to the editor state, not the node state. I didn't want to have to pass variables into the _hash_node() method. I could make it work that way if you'd like?

If you need to pass more than TreeItem then it's not worth it.

hpvb · 2024-11-28T15:55:18Z

Yes, last time I just checked all code that sets custom color or adds buttons, but there are other ways how TreeItems can be changed. There might be some tooltip changes I missed.

The things checked outside of the hash are things that are related to the editor state, not the node state. I didn't want to have to pass variables into the _hash_node() method. I could make it work that way if you'd like?

I'll go through _update_tooltip with a fine-tooth comb to make sure I have indeed caught everything.

If you need to pass more than TreeItem then it's not worth it.

I would have to, yes. I'll leave it as-is then.

hpvb · 2024-11-29T22:59:28Z

@KoBeWi I think I addressed your concerns and fixed the remaining bugs. I've also made the hash function smaller by relying on signals to update some aspects of the editor. These signals get emitted by Node itself, they must also be emitted if a script changes them. This claws back quite a lot of performance compared to the original non-hash based version.

Please be aware that quite a bit changed compared to last time!

The following things were tested:

Rename node / undo
Toggling unique names / undo
Make local / undo (verify icon/tooltip state)
Configuration warning / solve warning / undo
Adding to group / undo / Adding to group while connected / undo
Connecting a signal / undo/ Connecting signal while in a group / undo
Editable children / undo (verify color)
Create new node in an "editable children" scene, verify colors match master
Reparenting node / undo
Reparenting node via dialog, verified the colors and behavior against master
Reordering node / undo
Change editor theme / and back
popup_node_selector (your test case) Make sure that node selectability is updated appropriately
Creating connections via connections dialog, verified colors and behavior against master
Adding a script / undo
Toggle lock/unlock/undo, verify that lock icon appears in the scene tree
Verify that AnimationPlayer pin matches behavior on master
Set a node process mode / undo
Reparent node, cancel out of dialog, reparent a differnt node, validate that nodes do not stay red

new tests:

Change editor description, verify that tooltip changes.
Change group name group name does not change, this matches master behavior.
Create CollisonShape outside of a Body, move it into the body, verify that warnings clear.
Open connect dialog, close, add script to node, open, close, remove script from node, open,close verify that everything updates correctly.
Change type of node with children, ensure that that all warnings update correctly upwards and downwards in the tree.

hpvb · 2024-11-30T00:00:04Z

I think I see a couple more ways to optimize things a bit further. I'll make more changes, sorry for the extra noise.

a-johnston · 2024-11-30T05:24:33Z

Nice! I had been working on a similar change but with the holidays had to put it down. I'll go through this pr sometime this weekend and see if there are any changes which could be brought over.

a-johnston

Overall good changes! I feel skeptical about the value provided by some of the hashing/tree level dirty marking when we have granular access into specific node level changes. It feels like the old code was in part as prone to being slow as it was because the logic was so spread out that it was easiest and most reliable to fall back to a full tree update, and while the new code solves the full tree update part it retains IMO a lot of the complexity of the old code. Maybe I just missed some cases that required that approach.

Separately, on the core channel before we talked briefly about the sort of bad caching done within TreeItem and how that could be improved potentially with a skip list. Have you started on / planned to make those changes? If not, my branch also includes some other pretty minor improvements which I could rebase against this.

a-johnston · 2024-12-01T18:31:40Z

editor/gui/scene_tree_editor.cpp

-					break;
-				}
-			}
+		if (I) {


can this quit once !I since there are node parents outside of the editable tree but none past the editable root will have tree items? and more generally this can walk the tree items themselves rather than indexing into the map if the code applies any node added/removed handling prior

a-johnston · 2024-12-01T21:15:02Z

editor/gui/scene_tree_editor.cpp

+		if (IC) {
+			TreeItem *item = IC->value.item;
+
+			if (IC->value.item->get_parent() != parent) {


is there a reason you do this here rather than when node removed fires? it seems like it would simplify the code to just handle reordering here

a-johnston · 2024-12-01T21:37:51Z

editor/gui/scene_tree_editor.cpp

@@ -847,6 +1079,7 @@ void SceneTreeEditor::_test_update_tree() {
 	if (get_scene_node()) {
 		_compute_hash(get_scene_node(), hash);
 	}
+


is it worth keeping this and the _tree_changed handler around given the added/removed/rearranged callbacks give us the specific node/subtree and change? What case would fail for those other callbacks?

The new _tree_changed handler only dives into subtrees of changed nodes. This should fix your concern.

_update_tree() now doesn't much more work than is required.

a-johnston · 2024-12-01T21:41:54Z

editor/gui/scene_tree_editor.cpp

+		tree->clear();
+		node_cache.clear();


this is repeated; it might be worth adding a reset method

I'll add a reset method.

a-johnston · 2024-12-01T22:17:04Z

editor/gui/scene_tree_editor.cpp

+				TreeItem *item = I->value.item;
+				TreeItem *parent = item->get_parent();
+				if (parent) {
+					parent->remove_child(item);
+				}
+				node_cache.remove(I);


this looks like it leaks item

You are right, I fixed this in this iteration.

hpvb · 2024-12-02T02:24:05Z

@a-johnston @KoBeWi I've changed the way the updating system works, it is simpler now. As discussed it adds a new signal to node that fires only in the editor to make sure everything stays up to date.

I think this addresses everyone's concerns.

Note that there's a couple small issues left, which I will fix in a next push, assuming this design doesn't get shot down:

Reparenting node / undo <--- causes a warning in Tree.
AnimationPlayer pin <--- does not work

all of the other tests shown above work.

hpvb · 2024-12-02T02:43:58Z

Overall good changes! I feel skeptical about the value provided by some of the hashing/tree level dirty marking when we have granular access into specific node level changes. It feels like the old code was in part as prone to being slow as it was because the logic was so spread out that it was easiest and most reliable to fall back to a full tree update, and while the new code solves the full tree update part it retains IMO a lot of the complexity of the old code. Maybe I just missed some cases that required that approach.

Yeah, I realized this also. This now only updates the sub trees that were affected, but starting from the root of the tree each time, not updating any branches without dirty nodes in them. The reason for starting from "the top" every time is so I could retain all of the surrounding logic of _update_tree, and because basically every time you do anything you have to dirty the parent as well. Updating a small handful of extra parents that might not need it doesn't seem like a big problem.

Separately, on the core channel before we talked briefly about the sort of bad caching done within TreeItem and how that could be improved potentially with a skip list. Have you started on / planned to make those changes? If not, my branch also includes some other pretty minor improvements which I could rebase against this.

That was what I started on, but the Tree optimizations didn't really touch the performance issues of SceneTreeEditor. I do want to improve that as well, but in a separate PR. There's some optimizations I want to do to List<> and then move Tree to use List instead. That doesn't solve all problems, but some at least.

We now cache the Node*<>TreeItem* mapping in the SceneTreeEditor. This allows us to make targeted updates to the Tree used to display the scene tree in the editor. Previously on almost all changes to the scene tree the editor would rebuild the entire widget, causing a large number of deallocations an allocations. We now carefully manipulate the Tree widget in-situ saving a large number of these allocations. There is definitely more that could be done, but this is already a massive improvement. This fixes godotengine#83460

@bruvzg

Since this function now exists there seems to be little reason not to expose it. Discussed with @bruvzg.

a-johnston

I really like editor_state_changed change, I think it's a good way of hooking into this without adding more special cases.

The reason for starting from "the top" every time is so I could retain all of the surrounding logic of _update_tree, and because basically every time you do anything you have to dirty the parent as well. Updating a small handful of extra parents that might not need it doesn't seem like a big problem.

That's a good enough reason for me. I think I just didn't like the idea of needing that linear pass of each layer to re-find which nodes are dirty when we started with that information, but, even if there are ways around that, you've already been testing this against the worst case scenario scene tree for that cost and it's fine.

a-johnston · 2024-12-02T03:16:38Z

editor/gui/scene_tree_editor.cpp

-	item->set_text_overrun_behavior(0, TextServer::OVERRUN_NO_TRIMMING);
+	if (p_parent && dirty) {
+		bool reparent = false;
+		int current_node_index = p_node->get_index(false);


is it intentional not including internal children here and in other spots? I noticed the original code always included internal children but here we omit them sometimes, but not always, and even mix and match inclusion such as in setting child_index below

a-johnston · 2024-12-02T03:23:16Z

editor/gui/scene_tree_editor.cpp

+
+	} else {
+		// A parent might have moved/renamed.
+		item->set_metadata(0, p_node->get_path());


Do you think it would be worthwhile changing this metadata to be the Node * value itself rather than than the path? I did it on my branch but I don't know if it actually noticeably helped performance. Most locations end up just passing the value to get_node and the remaining use is _find which I think could be removed entirely. It would also of course avoid needing to update the path when that's the only change.

a-johnston · 2024-12-02T03:36:59Z

editor/gui/scene_tree_editor.cpp

+		TreeItem *p = I->value.item->get_parent();
+		if (p) {
+			p->remove_child(I->value.item);
+		}


the memdelete call handles this

I don't know why I didn't remember/mention this earlier but it might also be interesting to stage these removed TreeItems for deletion but not free them immediately so that when nodes are moved between subtrees the same TreeItem can be reused to avoid an alloc. I suppose it would be updated anyways though and at that point it's basically just instance pooling.

Mickeon · 2024-12-02T09:52:28Z

scene/main/node.h

+	// Editor only signal to keep the SceneTreeEditor in sync.
+	void _emit_editor_state_changed();


I believe you may also do this and keep the definition in node.cpp fully surrounded by TOOLS_ENABLED:

Suggested change

// Editor only signal to keep the SceneTreeEditor in sync.

void _emit_editor_state_changed();

#ifdef TOOLS_ENABLED

void _emit_editor_state_changed();

#else

void _emit_editor_state_changed() {};

#endif

But it's like, whatever. Same thing.

Mickeon · 2024-12-02T09:57:25Z

scene/main/node.cpp

+#ifdef TOOLS_ENABLED
+	// This is required for the SceneTreeEditor to properly keep track of when an update is needed.
+	// This signal might be expensive and not needed for anything outside of the editor.
+	if (Engine::get_singleton()->is_editor_hint()) {


A notable concern to me is that editor_state_changed will emit regardless of whether the node belongs in an edited scene or not. And this could be fixed...

Suggested change

if (Engine::get_singleton()->is_editor_hint()) {

if (is_part_of_edited_scene()) {

But at the same time, nothing of the signal hints that it would only emit these nodes specifically.

Mickeon · 2024-12-02T10:02:59Z

doc/classes/Node.xml

@@ -1068,6 +1068,11 @@
 				Emitted when the node's editor description field changed.
 			</description>
 		</signal>
+		<signal name="editor_state_changed">
+			<description>
+				Emitted when field relevant to the editor changed. Only emitted in the editor, to update the scene tree.


Once this is finalized, and if this signal would still exist... this is not good. We need to be more specific. For now:

Suggested change

Emitted when field relevant to the editor changed. Only emitted in the editor, to update the scene tree.

Emitted when an attribute of the node that is relevant to the editor changed. Only emitted in the editor.

hpvb added topic:editor performance cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Nov 26, 2024

hpvb requested review from a team as code owners November 26, 2024 01:39

arkology reviewed Nov 26, 2024

View reviewed changes

editor/gui/scene_tree_editor.cpp Outdated Show resolved Hide resolved

editor/gui/scene_tree_editor.cpp Outdated Show resolved Hide resolved

editor/gui/scene_tree_editor.cpp Outdated Show resolved Hide resolved

editor/gui/scene_tree_editor.cpp Outdated Show resolved Hide resolved

AThousandShips added this to the 4.x milestone Nov 26, 2024

bruvzg reviewed Nov 26, 2024

View reviewed changes

editor/gui/scene_tree_editor.h Outdated Show resolved Hide resolved

scene/gui/tree.cpp Show resolved Hide resolved

hpvb force-pushed the scene_tree_editor_performance branch 2 times, most recently from 6aefc0b to 9a25be0 Compare November 26, 2024 12:03

KoBeWi reviewed Nov 26, 2024

View reviewed changes

editor/gui/scene_tree_editor.cpp Outdated Show resolved Hide resolved

hpvb requested a review from a team as a code owner November 26, 2024 12:20

hpvb force-pushed the scene_tree_editor_performance branch from 431094f to 05519d4 Compare November 26, 2024 12:29

hpvb requested review from arkology, bruvzg and KoBeWi November 26, 2024 12:32

arkology approved these changes Nov 26, 2024

View reviewed changes

KoBeWi reviewed Nov 26, 2024

View reviewed changes

editor/gui/scene_tree_editor.cpp Outdated Show resolved Hide resolved

KoBeWi reviewed Nov 26, 2024

View reviewed changes

editor/gui/scene_tree_editor.cpp Outdated Show resolved Hide resolved

hpvb force-pushed the scene_tree_editor_performance branch from 05519d4 to d4d55d0 Compare November 26, 2024 13:20

KoBeWi reviewed Nov 26, 2024

View reviewed changes

editor/gui/scene_tree_editor.cpp Outdated Show resolved Hide resolved

KoBeWi reviewed Nov 26, 2024

View reviewed changes

editor/gui/scene_tree_editor.cpp Outdated Show resolved Hide resolved

hpvb force-pushed the scene_tree_editor_performance branch 2 times, most recently from c5fca34 to e076f60 Compare November 26, 2024 16:15

This comment was marked as outdated.

Sign in to view

hpvb mentioned this pull request Nov 26, 2024

Manipulating the scene tree gets progressively slower with more nodes added to the tree in the editor. #83460

Open

Mickeon reviewed Nov 26, 2024

View reviewed changes

doc/classes/TreeItem.xml Outdated Show resolved Hide resolved

hpvb force-pushed the scene_tree_editor_performance branch from e076f60 to 8851ae1 Compare November 26, 2024 17:18

hpvb force-pushed the scene_tree_editor_performance branch 2 times, most recently from 9a856ff to 8d7b5eb Compare November 28, 2024 01:18

KoBeWi reviewed Nov 28, 2024

View reviewed changes

editor/gui/scene_tree_editor.h Outdated Show resolved Hide resolved

KoBeWi reviewed Nov 28, 2024

View reviewed changes

editor/gui/scene_tree_editor.h Outdated Show resolved Hide resolved

KoBeWi reviewed Nov 28, 2024

View reviewed changes

editor/gui/scene_tree_editor.h Outdated Show resolved Hide resolved

KoBeWi reviewed Nov 28, 2024

View reviewed changes

editor/gui/scene_tree_editor.h Outdated Show resolved Hide resolved

KoBeWi reviewed Nov 28, 2024

View reviewed changes

editor/gui/scene_tree_editor.cpp Outdated Show resolved Hide resolved

KoBeWi reviewed Nov 28, 2024

View reviewed changes

editor/gui/scene_tree_editor.cpp Outdated Show resolved Hide resolved

hpvb force-pushed the scene_tree_editor_performance branch 2 times, most recently from c3a3a60 to 3c006fe Compare November 29, 2024 22:56

hpvb force-pushed the scene_tree_editor_performance branch from 3c006fe to 3dc7dea Compare November 29, 2024 23:43

a-johnston reviewed Dec 1, 2024

View reviewed changes

hpvb force-pushed the scene_tree_editor_performance branch from 3dc7dea to 22b2b64 Compare December 2, 2024 02:20

hpvb requested a review from a team as a code owner December 2, 2024 02:20

hpvb added 2 commits December 2, 2024 03:45

Expose clear_buttons to ClassDB

377141a

Since this function now exists there seems to be little reason not to expose it. Discussed with @bruvzg.

hpvb force-pushed the scene_tree_editor_performance branch from 22b2b64 to 377141a Compare December 2, 2024 02:46

a-johnston reviewed Dec 2, 2024

View reviewed changes

Mickeon reviewed Dec 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Scene Tree editor performance #99700

Improve Scene Tree editor performance #99700

hpvb commented Nov 26, 2024

hpvb commented Nov 26, 2024 •

edited

Loading

arkology left a comment

arkology left a comment •

edited

Loading

KoBeWi commented Nov 26, 2024

This comment was marked as outdated.

hpvb commented Nov 28, 2024

KoBeWi commented Nov 28, 2024

KoBeWi commented Nov 28, 2024

hpvb commented Nov 28, 2024 •

edited

Loading

KoBeWi commented Nov 28, 2024

hpvb commented Nov 28, 2024

hpvb commented Nov 29, 2024

hpvb commented Nov 30, 2024

a-johnston commented Nov 30, 2024

a-johnston left a comment

a-johnston Dec 1, 2024

a-johnston Dec 1, 2024

a-johnston Dec 1, 2024

hpvb Dec 2, 2024

a-johnston Dec 1, 2024

hpvb Dec 2, 2024

a-johnston Dec 1, 2024

hpvb Dec 2, 2024

hpvb commented Dec 2, 2024

hpvb commented Dec 2, 2024

a-johnston left a comment

a-johnston Dec 2, 2024

a-johnston Dec 2, 2024

a-johnston Dec 2, 2024 •

edited

Loading

Mickeon Dec 2, 2024

Mickeon Dec 2, 2024

Mickeon Dec 2, 2024

		// Editor only signal to keep the SceneTreeEditor in sync.
		void _emit_editor_state_changed();

	if (Engine::get_singleton()->is_editor_hint()) {
	if (is_part_of_edited_scene()) {

	Emitted when field relevant to the editor changed. Only emitted in the editor, to update the scene tree.
	Emitted when an attribute of the node that is relevant to the editor changed. Only emitted in the editor.

Improve Scene Tree editor performance #99700

Are you sure you want to change the base?

Improve Scene Tree editor performance #99700

Conversation

hpvb commented Nov 26, 2024

hpvb commented Nov 26, 2024 • edited Loading

arkology left a comment

Choose a reason for hiding this comment

arkology left a comment • edited Loading

Choose a reason for hiding this comment

KoBeWi commented Nov 26, 2024

This comment was marked as outdated.

hpvb commented Nov 28, 2024

KoBeWi commented Nov 28, 2024

KoBeWi commented Nov 28, 2024

hpvb commented Nov 28, 2024 • edited Loading

KoBeWi commented Nov 28, 2024

hpvb commented Nov 28, 2024

hpvb commented Nov 29, 2024

hpvb commented Nov 30, 2024

a-johnston commented Nov 30, 2024

a-johnston left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hpvb commented Dec 2, 2024

hpvb commented Dec 2, 2024

a-johnston left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-johnston Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hpvb commented Nov 26, 2024 •

edited

Loading

arkology left a comment •

edited

Loading

hpvb commented Nov 28, 2024 •

edited

Loading

a-johnston Dec 2, 2024 •

edited

Loading